-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: update eslint to v8 #18264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
feat: update eslint to v8 #18264
Conversation
b779b41 to
c8b1fd3
Compare
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
c8b1fd3 to
b2f7836
Compare
5b9d895 to
a6afd58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically this is considered a breaking change. Since this is an internal @sentry-internal/* package, we can release this in a non-breaking fashion
Before we merge this, let's check in with Electron, Lynx and React-Native SDK maintainers. They all use a 10.x version of @sentry-internal/eslint-config-sdk so we should make sure this change doesn't cause problems for them:
https://github.com/search?type=code&q=%40sentry-internal%2Feslint-config-sdk+owner%3Agetsentry+
I agree btw that we shouldn't have to stick to semver for this internal package. Let's just make sure we break no-one unexpectedly 😅
b2ce5c4 to
de82cc5
Compare
|
@Lms24 I just asked internally and it seems to be ok for the others |
de82cc5 to
bbc1816
Compare
bbc1816 to
e6cd95c
Compare
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Had some clarifying questions but nothing blocking. Thanks for taking care of this!
| ); | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this rule no longer applied? (no objections here since it's a test but we still want this rule in src files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a typescript eslint rule and it was never working, since it was a .js file. We didn't configure that plugin for JavaScript files
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable import/export */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: can we add a small explanation why we disable this here and in the other files? I guess once the false positive gets fixed, we'd get an "unused directive" lint error, so whoever runs into this will have some context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I'll add a comment everywhere directly
| winterCGHeadersToDict, | ||
| // eslint-disable-next-line deprecation/deprecation | ||
| anrIntegration, | ||
| // eslint-disable-next-line deprecation/deprecation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I guess it doesn't flag the deprecation here anymore because only some signatures of this API are deprecated 🤔
I think that's probably fine, was just curious if you think there might be another reason.
sentry-javascript/packages/node-core/src/integrations/anr/index.ts
Lines 268 to 279 in a1cc675
| export function disableAnrDetectionForCallback<T>(callback: () => T): T; | |
| export function disableAnrDetectionForCallback<T>(callback: () => Promise<T>): Promise<T>; | |
| /** | |
| * Temporarily disables ANR detection for the duration of a callback function. | |
| * | |
| * This utility function allows you to disable ANR detection during operations that | |
| * are expected to block the event loop, such as intensive computational tasks or | |
| * synchronous I/O operations. | |
| * | |
| * @deprecated The ANR integration has been deprecated. Use `eventLoopBlockIntegration` from `@sentry/node-native` instead. | |
| */ | |
| export function disableAnrDetectionForCallback<T>(callback: () => T | Promise<T>): T | Promise<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point - I'll investigate into that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite interesting - atm TypeScript does actually not inherit the jsdoc with the deprecated warning: microsoft/TypeScript#407
So it could be that the eslint plugin is now more like TypeScripts behavior and ignores that. I readded that: df47728 (#18264)
| /* eslint-disable jsdoc/require-jsdoc */ | ||
| /* eslint-disable max-lines */ | ||
| /* eslint-disable no-param-reassign */ | ||
| /* eslint-disable import/named */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: why do we need this new disable directive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import/export got a false positive, which affects most of our index barrel files (I didn't want to disable it entirely, so I added a global disable in each affected barrel file)
tracked here: import-js/eslint-plugin-import#703
| // these somehow fail with rollup.examples.config.mjs | ||
| files: ['*.mjs'], | ||
| rules: { | ||
| 'import/named': 'off', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: any reason for this?
This upgrades our internal eslint config to use v8. Theoretically this is considered a breaking change. Since this is an internal
@sentry-internal/*package, we can release this in a non-breaking fashionWith v8 couple of changes came. Some of them are affecting us, which required file changes:
Merge checks